[MR updates] use client arg for resource calls - rest of the tests#985
[MR updates] use client arg for resource calls - rest of the tests#985dbasunag merged 3 commits intoopendatahub-io:mainfrom
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/hold', '/verified', '/cherry-pick', '/lgtm', '/wip', '/build-push-pr-image'} |
📝 WalkthroughWalkthroughThe PR threads an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/model_registry/rest_api/test_multiple_mr.py (1)
45-49: Consider removing unusedmodel_registry_instanceparameter from signature.The
model_registry_instanceparameter is declared but never used in the test body, as flagged by Ruff. Since the fixture is already included in@pytest.mark.usefixtures(line 40), having it in the function signature is redundant unless you plan to use the fixture's return value in the test.🔎 Proposed cleanup
def test_validate_multiple_model_registry( self: Self, - model_registry_instance: list[ModelRegistry], model_registry_namespace: str, admin_client: DynamicClient, ):
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/model_registry/model_catalog/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/rbac/conftest.pytests/model_registry/rest_api/conftest.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rest_api/test_multiple_mr.py
🧰 Additional context used
🧬 Code graph analysis (6)
tests/model_registry/rest_api/conftest.py (3)
tests/conftest.py (1)
admin_client(79-80)tests/model_registry/conftest.py (1)
model_registry_namespace(76-77)utilities/resources/model_registry_modelregistry_opendatahub_io.py (1)
ModelRegistry(9-94)
tests/model_registry/rest_api/test_multiple_mr.py (3)
tests/model_registry/conftest.py (2)
model_registry_instance(81-117)model_registry_namespace(76-77)utilities/resources/model_registry_modelregistry_opendatahub_io.py (1)
ModelRegistry(9-94)tests/conftest.py (1)
admin_client(79-80)
tests/model_registry/negative_tests/conftest.py (1)
tests/conftest.py (1)
admin_client(79-80)
tests/model_registry/rest_api/test_model_registry_rest_api.py (3)
tests/conftest.py (1)
admin_client(79-80)tests/model_registry/conftest.py (1)
model_registry_instance(81-117)utilities/resources/model_registry_modelregistry_opendatahub_io.py (1)
ModelRegistry(9-94)
tests/model_registry/model_catalog/conftest.py (2)
tests/conftest.py (1)
admin_client(79-80)tests/model_registry/conftest.py (1)
model_registry_namespace(76-77)
tests/model_registry/negative_tests/test_model_registry_creation_negative.py (1)
tests/conftest.py (1)
admin_client(79-80)
🪛 Ruff (0.14.10)
tests/model_registry/rest_api/test_multiple_mr.py
46-46: Unused method argument: model_registry_instance
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (13)
tests/model_registry/model_catalog/conftest.py (2)
46-46: Correct return type annotation.The return type annotation now accurately reflects the yield-based fixture pattern. This improves type safety and IDE support.
335-337: Correct return type annotation and proper client parameter threading.The changes correctly:
- Update the return type to
Generator[dict[str, Any], None, None]to match the yield pattern at line 357- Add
admin_clientparameter for explicit client passing to ConfigMap constructor (line 339)- Add
model_registry_namespaceparameter for proper namespace scopingThese changes align with the PR's objective to use client arguments for resource calls and improve type safety.
tests/model_registry/negative_tests/conftest.py (1)
169-173: LGTM! Admin client properly threaded through deployment deletion.The fixture signature and Deployment constructor are correctly updated to use the admin_client parameter, ensuring proper permissions for resource deletion operations.
tests/model_registry/rest_api/conftest.py (2)
143-174: LGTM! ModelRegistry correctly instantiated with admin client.The admin_client is properly passed to the ModelRegistry constructor on line 158, ensuring the secure MySQL and MR deployment uses the correct authenticated client context.
364-376: LGTM! Deployment lookup correctly uses admin client.The fixture signature and Deployment constructor are properly updated to use admin_client, ensuring proper permissions when retrieving the default postgres deployment's match labels.
tests/model_registry/rbac/conftest.py (3)
142-176: LGTM! Parametrized Secret and PVC fixtures correctly use admin client.Both
db_secret_parametrizedanddb_pvc_parametrizedfixtures are properly updated to accept and thread the admin_client through to their respective resource constructors, maintaining consistency with the PR's objective.
180-222: LGTM! Parametrized Service and Deployment fixtures correctly use admin client.Both fixtures are properly updated to thread admin_client through to resource constructors. The addition of
wait_for_replicas(deployed=True)on lines 219-220 ensures deployments are ready before tests proceed, which is a good reliability practice.
226-242: LGTM! ModelRegistry parametrized fixture correctly uses admin client.The fixture properly threads admin_client to the ModelRegistry constructor on line 232, ensuring all MR instances are created with the correct authenticated client context. The wait conditions and logging enhance test reliability.
tests/model_registry/negative_tests/test_model_registry_creation_negative.py (2)
4-5: LGTM! Import properly added for DynamicClient type annotation.The import is correctly placed and necessary for the type hint added to the test method signature.
33-71: LGTM! Test method correctly uses admin client for negative test scenario.The test method properly receives admin_client as a fixture parameter and passes it to the ModelRegistry constructor. The negative test logic remains intact, correctly expecting a ForbiddenError when attempting to create a ModelRegistry in an unauthorized namespace.
tests/model_registry/rest_api/test_model_registry_rest_api.py (2)
169-182: LGTM! Test method correctly uses admin client and improved type hints.The test method properly receives admin_client via fixture injection and passes it to the ModelRegistry constructor. The added type hint on line 172 improves code clarity. The test logic for validating API version remains unchanged.
184-191: LGTM! Type hint added for improved code clarity.The type hint for
model_registry_instanceimproves code clarity. This test doesn't require admin_client as it only reads the ModelRegistry spec without creating or modifying resources.tests/model_registry/rest_api/test_multiple_mr.py (1)
51-56: LGTM! Admin client correctly integrated.The ModelRegistry instances are now properly constructed with
client=admin_client, which aligns with the PR objective of threading the admin client through resource calls.
dbasunag
force-pushed
the
update_client_mr_2
branch
from
517fb07 to
1879d85
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/model_registry/model_registry/negative_tests/test_model_registry_creation_negative.py (1)
55-70: Fix the parameter name: useclient=instead ofclient_token=.Line 56 incorrectly uses
client_token=admin_clientwhen instantiating ModelRegistry. Based on the ModelRegistry class signature and all other usages in this PR, it should beclient=admin_client.🐛 Proposed fix
with ModelRegistry( - client_token=admin_client, + client=admin_client, name=MR_INSTANCE_NAME,
🤖 Fix all issues with AI agents
In
@tests/model_registry/model_registry/negative_tests/test_model_registry_creation_negative.py:
- Line 6: The file contains an unused import "from tests.conftest import
admin_client" that conflicts with the pytest fixture parameter admin_client used
in the test function; remove that import line so the test relies on the
pytest-injected fixture (referenced as admin_client in the test function) to
eliminate the linter warning and redefinition.
In
@tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py:
- Line 5: Remove the incorrect import of the pytest fixture: delete the line
importing admin_client from tests.conftest in the test module; rely on pytest to
inject the admin_client fixture via test function parameter (as used in tests
like the one at line where admin_client is declared as a test argument) and
ensure no other code references expect a direct import of admin_client.
- Around line 172-173: The parameter list in the test function has a
hanging-indent misalignment: align the continuation parameter
"model_registry_instance: list[ModelRegistry]" with the previous parameter
"admin_client: DynamicClient" (i.e., same indentation level or under the opening
parenthesis per project style) so the two parameters form a properly aligned
multi-line parameter list; update the indentation for the symbol
model_registry_instance accordingly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/model_registry/model_catalog/conftest.pytests/model_registry/model_registry/negative_tests/conftest.pytests/model_registry/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/model_registry/rbac/conftest.pytests/model_registry/model_registry/rest_api/conftest.pytests/model_registry/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/model_registry/rest_api/test_multiple_mr.py
🧰 Additional context used
🧬 Code graph analysis (5)
tests/model_registry/model_registry/rbac/conftest.py (2)
tests/conftest.py (1)
admin_client(79-80)utilities/resources/model_registry_modelregistry_opendatahub_io.py (1)
ModelRegistry(9-94)
tests/model_registry/model_registry/rest_api/conftest.py (3)
tests/conftest.py (1)
admin_client(79-80)tests/model_registry/conftest.py (1)
model_registry_namespace(57-58)utilities/resources/model_registry_modelregistry_opendatahub_io.py (1)
ModelRegistry(9-94)
tests/model_registry/model_registry/rest_api/test_multiple_mr.py (3)
tests/conftest.py (1)
admin_client(79-80)tests/model_registry/conftest.py (2)
model_registry_instance(284-320)model_registry_namespace(57-58)utilities/resources/model_registry_modelregistry_opendatahub_io.py (1)
ModelRegistry(9-94)
tests/model_registry/model_registry/negative_tests/test_model_registry_creation_negative.py (1)
tests/conftest.py (1)
admin_client(79-80)
tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py (3)
tests/conftest.py (1)
admin_client(79-80)tests/model_registry/conftest.py (1)
model_registry_instance(284-320)utilities/resources/model_registry_modelregistry_opendatahub_io.py (1)
ModelRegistry(9-94)
🪛 Flake8 (7.3.0)
tests/model_registry/model_registry/negative_tests/test_model_registry_creation_negative.py
[error] 6-6: 'tests.conftest.admin_client' imported but unused
(F401)
[error] 35-35: redefinition of unused 'admin_client' from line 6
(F811)
tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py
[error] 5-5: 'tests.conftest.admin_client' imported but unused
(F401)
[error] 172-172: redefinition of unused 'admin_client' from line 5
(F811)
[error] 173-173: continuation line unaligned for hanging indent
(E131)
🪛 Ruff (0.14.10)
tests/model_registry/model_registry/rest_api/test_multiple_mr.py
45-45: Unused method argument: model_registry_instance
(ARG002)
tests/model_registry/model_registry/negative_tests/test_model_registry_creation_negative.py
35-35: Redefinition of unused admin_client from line 6
(F811)
tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py
172-172: Redefinition of unused admin_client from line 5
(F811)
🔇 Additional comments (14)
tests/model_registry/model_catalog/conftest.py (1)
47-47: Correct type annotation for generator-based fixture.The return type now accurately reflects that this fixture uses
yield(line 90), aligning with similar fixtures in the file such asupdated_catalog_config_mapandupdate_configmap_data_add_model. This improves type safety and consistency.tests/model_registry/model_registry/rest_api/conftest.py (3)
90-120: LGTM! Consistent admin_client usage in patch_invalid_ca.The fixture correctly accepts
admin_clientand threads it through to the ConfigMap constructor.
142-174: LGTM! Proper admin_client integration for secure MySQL deployment.The
deploy_secure_mysql_and_mrfixture correctly passesclient=admin_clientto ModelRegistry construction, aligning with the PR's objective.
363-373: LGTM! Deployment constructor receives admin_client correctly.The fixture signature and Deployment instantiation follow the established pattern.
tests/model_registry/model_registry/rbac/conftest.py (5)
141-154: LGTM! Parametrized secret fixture updated correctly.The
admin_clientparameter is properly threaded to Secret construction.
157-172: LGTM! Parametrized PVC fixture updated correctly.The
admin_clientparameter is properly threaded to PersistentVolumeClaim construction.
175-188: LGTM! Parametrized service fixture updated correctly.The
admin_clientparameter is properly threaded to Service construction.
191-210: LGTM! Parametrized deployment fixture updated correctly.The
admin_clientparameter is properly threaded to Deployment construction.
213-230: LGTM! ModelRegistry instances receive admin_client correctly.The fixture properly instantiates ModelRegistry with
client=admin_client, maintaining consistency with the PR pattern.tests/model_registry/model_registry/negative_tests/conftest.py (2)
113-141: LGTM! Deployment construction uses admin_client.The fixture correctly passes
client=admin_clientto the Deployment constructor.
171-177: LGTM! delete_mr_deployment fixture updated correctly.The fixture properly accepts
admin_clientand uses it when constructing the Deployment.tests/model_registry/model_registry/rest_api/test_multiple_mr.py (1)
44-52: LGTM! Test method correctly uses admin_client.The test properly accepts
admin_clientand passes it to ModelRegistry construction withclient=admin_client.Note: The static analysis warning about
model_registry_instancebeing unused is a false positive—it's used indirectly through pytest's fixture mechanism (Line 40 in@pytest.mark.usefixtures).tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py (2)
170-183: LGTM! Client parameter threading implemented correctly.The addition of
admin_clientparameter and passing it to theModelRegistryconstructor aligns with the PR objective to use the client argument for resource calls. The type annotation formodel_registry_instancealso improves code clarity.
185-192: LGTM! Type annotation improves clarity.The type annotation for
model_registry_instanceis consistent with the changes in other test methods and enhances code readability.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/model_registry/model_registry/negative_tests/test_model_registry_creation_negative.py (1)
54-69: Fix incorrect parameter name - useclient=instead ofclient_token=.Line 55 passes
admin_client(aDynamicClient) to theclient_tokenparameter, but the correct parameter name isclient. This is confirmed by the pattern used consistently in other files (e.g.,test_multiple_mr.pyLine 52 usesclient=admin_client).The
current_client_token: strparameter on Line 35 appears to be the actual token and is now unused.🐛 Proposed fix
with ModelRegistry( - client_token=admin_client, + client=admin_client, name=MR_INSTANCE_NAME, namespace=model_registry_namespace_for_negative_tests.name,
🧹 Nitpick comments (1)
tests/model_registry/model_registry/rest_api/test_multiple_mr.py (1)
44-49: Consider moving unused parameter to@pytest.mark.usefixtures.The
model_registry_instanceparameter (Line 47) is declared but never referenced in the test body. While the fixture is needed for its setup side effects, consider moving it to the@pytest.mark.usefixturesdecorator on Line 37 to make the intent clearer and eliminate the static analysis warning.♻️ Proposed refactor
@pytest.mark.usefixtures( "updated_dsc_component_state_scope_session", "model_registry_metadata_db_resources", "model_registry_instance", ) class TestModelRegistryMultipleInstances: @pytest.mark.smoke def test_validate_multiple_model_registry( self: Self, admin_client: DynamicClient, - model_registry_instance: list[ModelRegistry], model_registry_namespace: str, ):
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/model_registry/model_registry/negative_tests/conftest.pytests/model_registry/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/model_registry/rbac/conftest.pytests/model_registry/model_registry/rest_api/conftest.pytests/model_registry/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/model_registry/rest_api/test_multiple_mr.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py
- tests/model_registry/model_registry/rbac/conftest.py
- tests/model_registry/model_registry/rest_api/conftest.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/model_registry/model_registry/negative_tests/test_model_registry_creation_negative.py (1)
tests/conftest.py (1)
admin_client(79-80)
tests/model_registry/model_registry/negative_tests/conftest.py (1)
tests/conftest.py (1)
admin_client(79-80)
tests/model_registry/model_registry/rest_api/test_multiple_mr.py (3)
tests/conftest.py (1)
admin_client(79-80)tests/model_registry/conftest.py (2)
model_registry_instance(284-320)model_registry_namespace(57-58)utilities/resources/model_registry_modelregistry_opendatahub_io.py (1)
ModelRegistry(9-94)
🪛 Ruff (0.14.10)
tests/model_registry/model_registry/rest_api/test_multiple_mr.py
47-47: Unused method argument: model_registry_instance
(ARG002)
🔇 Additional comments (3)
tests/model_registry/model_registry/rest_api/test_multiple_mr.py (1)
51-56: LGTM!The
ModelRegistryinstantiation correctly usesclient=admin_client, following the established pattern across the codebase.tests/model_registry/model_registry/negative_tests/conftest.py (2)
121-141: LGTM!The
admin_clientparameter is correctly threaded through to theDeploymentconstructor usingclient=admin_client(Line 122). The fixture properly manages the deployment lifecycle.
172-176: LGTM!The
delete_mr_deploymentfixture correctly acceptsadmin_clientand passes it to theDeploymentconstructor usingclient=admin_client(Line 175).
dbasunag
force-pushed
the
update_client_mr_2
branch
from
40e8be0 to
a9549d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/model_registry/model_registry/rest_api/test_multiple_mr.py (1)
45-57: Consider using themodel_registry_instancefixture parameter directly.The
model_registry_instanceparameter is declared but not used in the test body. Instead, the test recreatesModelRegistryobjects withensure_exists=True(lines 51-56). While this verifies the instances exist, it's redundant since the fixture already provides the created instances.Consider refactoring to use the fixture parameter directly:
♻️ Proposed refactor
def test_validate_multiple_model_registry( self: Self, admin_client: DynamicClient, model_registry_instance: list[ModelRegistry], model_registry_namespace: str, ): - for num in range(0, NUM_RESOURCES["num_resources"]): - mr = ModelRegistry( - client=admin_client, - name=f"{MR_INSTANCE_BASE_NAME}{num}", - namespace=model_registry_namespace, - ensure_exists=True, - ) + for mr in model_registry_instance: LOGGER.info(f"{mr.name} found")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/model_registry/model_catalog/conftest.pytests/model_registry/model_registry/negative_tests/conftest.pytests/model_registry/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/model_registry/rbac/conftest.pytests/model_registry/model_registry/rest_api/conftest.pytests/model_registry/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/model_registry/rest_api/test_multiple_mr.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py
- tests/model_registry/model_registry/negative_tests/conftest.py
- tests/model_registry/model_catalog/conftest.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/model_registry/model_registry/negative_tests/test_model_registry_creation_negative.py (1)
tests/conftest.py (1)
admin_client(79-80)
tests/model_registry/model_registry/rest_api/test_multiple_mr.py (3)
tests/conftest.py (1)
admin_client(79-80)tests/model_registry/conftest.py (2)
model_registry_instance(284-320)model_registry_namespace(57-58)utilities/resources/model_registry_modelregistry_opendatahub_io.py (1)
ModelRegistry(9-94)
🪛 Ruff (0.14.10)
tests/model_registry/model_registry/rest_api/test_multiple_mr.py
47-47: Unused method argument: model_registry_instance
(ARG002)
🔇 Additional comments (5)
tests/model_registry/model_registry/rbac/conftest.py (1)
142-238: LGTM! Consistent threading of admin_client through parametrized fixtures.All five parametrized fixtures (
db_secret_parametrized,db_pvc_parametrized,db_service_parametrized,db_deployment_parametrized, andmodel_registry_instance_parametrized) now correctly acceptadmin_client: DynamicClientand passclient=admin_clientto their respective resource constructors. This ensures consistent client usage across the RBAC test suite.tests/model_registry/model_registry/rest_api/test_multiple_mr.py (1)
51-56: Approve admin_client threading.The addition of
admin_clientparameter and its use inModelRegistryconstruction aligns with the PR objective to use client arg for resource calls.tests/model_registry/model_registry/negative_tests/test_model_registry_creation_negative.py (1)
17-17: LGTM! Consistent admin_client usage in negative test.The changes correctly:
- Import
DynamicClient(line 17)- Add
admin_clientparameter to the test method (line 34)- Pass
client=admin_clienttoModelRegistryconstructor (line 55)This aligns with the PR objective to thread admin_client through model registry resource calls.
Also applies to: 34-34, 54-70
tests/model_registry/model_registry/rest_api/conftest.py (2)
364-376: LGTM! Fixture correctly updated with admin_client.The
model_registry_default_postgres_deployment_match_labelfixture now acceptsadmin_client: DynamicClientand correctly passesclient=admin_clientto theDeploymentconstructor (line 371). This ensures the Deployment object uses the provided admin client for cluster interactions.Note: The AI summary incorrectly states that a ConfigMap fixture's return type changed to a generator. This fixture returns a
dict[str, str]directly (line 376) and is not a generator.
105-109: Approve admin_client threading in ConfigMap and ModelRegistry fixtures.The fixtures correctly pass
client=admin_clientto:
ConfigMapconstructor inpatch_invalid_ca(line 105)ModelRegistryconstructor indeploy_secure_mysql_and_mr(line 158)ConfigMapconstructor inca_configmap_for_test(line 223)This ensures consistent admin client usage across REST API test fixtures.
Also applies to: 158-158, 223-227
|
Status of building tag latest: success. |
4 participants
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.